ボーリングプログラム作成 by jepe-w · Pull Request #3 · jepe-w/ruby-practices

jepe-w · 2024-02-29T04:38:09Z

No description provided.

JunichiIto

全体的に修正が必要な箇所が多数あるので、提出物のページにコメントします。

また、bug_cafeへのシンボリックリンク？がdiffに含まれています。
これも不要なので削除してください。

JunichiIto · 2024-03-06T23:46:06Z

03.bowling/y-bowling.rb

+end
+
+
+p point


ファイルの末尾に赤丸が出ないようにしてください。
https://docs.komagata.org/5676#crlf

あと、pメソッドはデバッグ目的で使うことが多いメソッドです。
通常のターミナル出力はputsまたはprintを使ってください。

JunichiIto

コメントしました！

また、前回コメントした「また、bug_cafeへのシンボリックリンク？がdiffに含まれています。
これも不要なので削除してください。」が対応できてないようなので再確認をお願いします。

JunichiIto · 2024-03-11T00:27:16Z

03.bowling/y-bowling.rb

+#!/usr/bin/env ruby
+
+score = ARGV[0]
+score = score.split(',')


再代入を避け、別の変数名を付けましょう
https://bootcamp.fjord.jp/pages/avoid-re-assign

ありがとうございます！
複雑なコードを書くようになる前に知れてよかったです✊

JunichiIto · 2024-03-11T00:28:47Z

03.bowling/y-bowling.rb

+  frames[9] += frames[10]
+  frames.pop


Suggested change

frames[9] += frames[10]

frames.pop

frames[9] += frames.pop

でよいですね

JunichiIto · 2024-03-11T00:30:20Z

03.bowling/y-bowling.rb

+end
+
+point = 0
+count = 0


each_with_indexメソッドを使えばcount変数をなくせるはずですー

ありがとうございます！
こういうのをパット考えれるようになりたい。。

JunichiIto · 2024-03-11T00:31:39Z

03.bowling/y-bowling.rb

+count = 0
+frames.each do |frame|
+  point += if frame.length == 2 && frame[0] == 10
+             if frames[count + 1].length == 3


next_frame = frames[count + 1]

のようにして変数に入れるとコードが読みやすくなります。
説明変数、説明用変数、みたいなキーワードでネットを検索してみてください。

ありがとうございます！
変数の命名が今の自分の課題の一つだと思っていますが、
今回の件をきっかけに良い記事を見つけて、少し変数の命名に幅が広がった気がします。
日々勉強を重ねて今後はこう言った小さいけれどとても大切なことや気遣いを意識しながら書いていきたいです。

JunichiIto · 2024-03-11T00:34:56Z

03.bowling/y-bowling.rb

+frames.each do |frame|
+  point += if frame.length == 2 && frame[0] == 10
+             if frames[count + 1].length == 3
+               10 + frames[count + 1][0] + frames[count + 1][1]


Suggested change

10 + frames[count + 1][0] + frames[count + 1][1]

10 + frames[count + 1][0..1].sum

みたいに書くこともできます

ありがとうございます。
採用させていただきました！笑

JunichiIto · 2024-03-11T00:35:52Z

03.bowling/y-bowling.rb

+             elsif frames[count + 1][0] == 10
+               10 + 10 + frames[count + 2][0]
+             else
+               10 + frames[count + 1][0] + frames[count + 1][1]


Suggested change

10 + frames[count + 1][0] + frames[count + 1][1]

10 + frames[count + 1].sum

elseに入ってるならlengthが2であることは決定しているので、こんなふうに書けますね

JunichiIto

コメントしました！

JunichiIto · 2024-03-12T23:22:05Z

03.bowling/y-bowling.rb

+# frozen_string_literal: true
+
+score = ARGV[0]
+split_score = score.split(',')


配列なので複数形にした方がわかりやすいです。
あと、splitは動詞なので、split_scoreだと「スコアを分割する」というメソッドに見えます。
https://qiita.com/jnchito/items/459d58ba652bf4763820

splitした、という意味なら splited_scores みたいな名前になりますが、そもそもここはシンプルに

Suggested change

split_score = score.split(',')

scores = ARGV[0].split(',')

で良さそうです

なるほどです。
こういう単純そうに感じることでも言われてようやく気が付きます。。🥲
思考の柔軟性がたりないのでしょうか、、

思考の柔軟性がたりないのでしょうか、、

最初から完璧に書ける人はいないので大丈夫です！
僕なんか20年プログラマやってますからね。単純に経験値だけ見てもきっと雲泥の差があると思います。
（同じコードを見ても、jepeさんには見えない何かが僕にはたくさん見えてるはず）

過去に同じようなテーマでブログを書いたので参考にしてみてください。
https://blog.jnito.com/entry/2020/08/24/083030

リーダブルコードやCODE COMPLETEみたいに「読みやすくてわかりやすいコードの書き方」に特化した技術書もあるので、そうした本を読んでみるのもお勧めです！

JunichiIto · 2024-03-12T23:23:08Z

03.bowling/y-bowling.rb

+    shots << if s == 'X'
+               10
+             else
+               s.to_i
+             end


Suggested change

shots << if s == 'X'

10

else

s.to_i

end

shots << (s == 'X' ? 10 : s.to_i)

三項演算子の方がすっきりしそうです

さらにいうと、ロジック的にはここまで短くできるかも？

split_score.each do |s| shots << (s == 'X' ? 10 : s.to_i) shots << 0 if shots.length < 18 && s == 'X' end

３項演算子は理解できていたつもりになっていましたが、このように書けるのは正直思いもしませんでした。
こういったテクニックを教えていただけるのはとても良い経験となります😁

JunichiIto · 2024-03-12T23:29:00Z

03.bowling/y-bowling.rb

+frames = []
+shots.each_slice(2) do |s|
+  frames << s
+end


Suggested change

frames = []

shots.each_slice(2) do |s|

frames << s

end

frames = shots.each_slice(2).to_a

でOKです

JunichiIto · 2024-03-12T23:33:48Z

03.bowling/y-bowling.rb

+point = 0
+frames.each_with_index do |frame, i|


Suggested change

point = 0

frames.each_with_index do |frame, i|

point = frames.each_with_index.sum do |frame, i|

sumメソッドを使ってpoint +=をなくしてみましょう

JunichiIto · 2024-03-12T23:37:23Z

03.bowling/y-bowling.rb

+point = 0
+frames.each_with_index do |frame, i|
+  next_frame = frames[i + 1]
+  not_last_frame = frame.length == 2


Suggested change

not_last_frame = frame.length == 2

not_last_frame = i != 9

とした方がコードの意図が明確になりそうです

JunichiIto

いくつかコメントしましたが大きな問題ではないのでこれでOKです！🙆‍♂️

JunichiIto · 2024-03-13T23:36:44Z

03.bowling/y-bowling.rb

+frames = shots.each_slice(2).to_a
+
+frames[9] += frames.pop if frames.length == 11
+point = frames.each_with_index.sum do |frame, i|


Suggested change

frames = shots.each_slice(2).to_a

frames[9] += frames.pop if frames.length == 11

point = frames.each_with_index.sum do |frame, i|

frames = shots.each_slice(2).to_a

frames[9] += frames.pop if frames.length == 11

point = frames.each_with_index.sum do |frame, i|

こんなふうに改行した方が処理のグルーピングとしてわかりやすいように思いました

JunichiIto · 2024-03-13T23:40:39Z

03.bowling/y-bowling.rb

+  next_frame = frames[i + 1]
+  not_last_frame = i != 9
+  strilke = frame[0] == 10
+  spare = frame.sum == 10
+
+  if not_last_frame && strilke
+    if next_frame.length == 3
+      10 + next_frame[0..1].sum
+    elsif next_frame[0] == 10
+      10 + 10 + frames[i + 2][0]
+    else
+      10 + next_frame.sum
+    end
+  elsif not_last_frame && spare
+    frame.sum + next_frame[0]
+  else
+    frame.sum
+  end


あくまで参考情報ですが、こんなふうに書くこともできます。

Suggested change

next_frame = frames[i + 1]

not_last_frame = i != 9

strilke = frame[0] == 10

spare = frame.sum == 10

if not_last_frame && strilke

if next_frame.length == 3

10 + next_frame[0..1].sum

elsif next_frame[0] == 10

10 + 10 + frames[i + 2][0]

else

10 + next_frame.sum

end

elsif not_last_frame && spare

frame.sum + next_frame[0]

else

frame.sum

end

next frame.sum if i == 9

next_frame = frames[i + 1]

strilke = frame[0] == 10

spare = frame.sum == 10

if strilke

if i == 8

10 + next_frame[0..1].sum

elsif next_frame[0] == 10

10 + 10 + frames[i + 2][0]

else

10 + next_frame.sum

end

elsif spare

frame.sum + next_frame[0]

else

frame.sum

end

ポイントは以下の通りです。

最終フレームだけ特別扱いして（next frame.sum if i == 9）、21行目、29行目の条件分岐をシンプルにする

if next_frame.length == 3 が真になるのは9フレーム目しかないので、if i == 8 としてしまう（これは好みの問題かも）

なるほど、確かに最終フレームを特別扱いするだけで、
そのほかのコードも理解しやすいように書けるようになっているのですね

特別な処理とそうでない処理を切り分けるというのはいろんなところで使えそうなので
使っていこうと思います！

jepe-w added 2 commits February 29, 2024 13:29

ボーリングプログラム作成

03b23a2

require debugを削除

3b3702d

JunichiIto reviewed Mar 6, 2024

View reviewed changes

レビューでの指摘箇所改善　v1

871a54a

JunichiIto reviewed Mar 11, 2024

View reviewed changes

jepe-w added 3 commits March 12, 2024 08:49

不要なファイルの削除

6f95c4c

不要ファイルの削除

da48e78

レビューでの指摘箇所改善

baf1556

JunichiIto reviewed Mar 12, 2024

View reviewed changes

レビュー指摘箇所の改善　v2

69c20ec

JunichiIto approved these changes Mar 13, 2024

View reviewed changes

	10 + frames[count + 1][0] + frames[count + 1][1]
	10 + frames[count + 1][0..1].sum

	10 + frames[count + 1][0] + frames[count + 1][1]
	10 + frames[count + 1].sum

	point = 0
	frames.each_with_index do \|frame, i\|
	point = frames.each_with_index.sum do \|frame, i\|

Conversation

jepe-w commented Feb 29, 2024

Uh oh!

JunichiIto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunichiIto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunichiIto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunichiIto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants